Kael's Contributions #3
Conversation
kael-shipman
force-pushed
the
ft/hierarchical-tags-kael
branch
from
5aeef74 to
657a969
Compare
| if (hierarchicalTags) { | ||
| // If the `hierarchicalTags` option is set, we want to break down the tags into a deep | ||
| // hierarchy. We're using a "raw" object for cleanliness here, but later we'll convert that | ||
| // into an immutable map. Here are the types we're dealing with: | ||
| // | ||
| // const operationTagsRaw: TagMap; | ||
| // type TagMap = { [TagName: string]: TagData }; | ||
| // type TagData = { | ||
| // canonicalName: string; | ||
| // data: TagInfoAndOperations | null; | ||
| // childTags: TagMap; | ||
| // } | ||
| // TODO: Explicitly define TagInfoAndOperations | ||
|
|
||
| const operationTagsRaw = {}; | ||
|
|
||
| // For each raw tag.... | ||
| taggedOps.map((tagObj, tagName) => { | ||
| // Split the raw tag name into parts | ||
| const parts = tagName.split(tagSplitterChar); | ||
|
|
||
| // Set a pointer for use in traversing the hierarchy | ||
| let current = operationTagsRaw; | ||
|
|
||
| // Iterate through the parts defined by this tag | ||
| for (let i = 0; i < parts.length; i++) { | ||
| const part = parts[i]; | ||
|
|
||
| // If there's no object defined for the current part, define one with just childTags as an | ||
| // empty set | ||
| if (current[part] === undefined) { | ||
| // Compose canonical name from parts up to this point | ||
| const canonicalName = parts.reduce( | ||
| (name, p, j) => ((j > i) ? name : name.concat([p])), | ||
| [] | ||
| ).join("|"); | ||
| current[part] = { | ||
| canonicalName, | ||
| data: null, | ||
| childTags: {} | ||
| } | ||
| } | ||
|
|
||
| { taggedOps.size < 1 ? <h3> No operations defined in spec! </h3> : null } | ||
| </div> | ||
| ) | ||
| } | ||
| // If this is the last part, set data on this object | ||
| if (i === parts.length - 1) { | ||
| current[part].data = tagObj; | ||
| } | ||
|
|
||
| // Move to the next level of the hierarchy before looping around | ||
| current = current[part].childTags; | ||
| } | ||
| }); |
There was a problem hiding this comment.
This is what I would encapsulates into a selector in because manipulating data from the state is not a components concern.
There was a problem hiding this comment.
Yes, fair comment. It's good to think about and enforce separation of concerns. I tend to optimize for keeping things simple, but in this case perhaps the added complexity of moving logic around the codebase is worth it. Plus, who knows who else may want to use a set of hierarchicalized tags :), and they'd be able to do so if this were extracted into a selector. I'll work on it.
There was a problem hiding this comment.
Hm... But now we run into the issue you were having earlier with filtering. Do you think it's more appropriate to create a filteredTagHierarchy selector? Or maybe to create a helper function that hierarchicalizes tags.
Making a spec selector that knows about a filter from the layout section seems like it's blurring the lines a little bit. I guess that means I'm putting my vote down in favor of a hierarchicalizeOperationTags helper function. That way you can use a spec selector to get all tags (flat), you can use a layout filter to filter them, then you can use a helper function to turn them into a hierarchy.
This seems like it honors the "well-organized box of simple tools" approach, rather than the "one large, complex tool" approach, and I think I like that.
There was a problem hiding this comment.
Yep you could just integrate this logic into src/core/plugins/spec/selectors.js
There was a problem hiding this comment.
I would prefer the approach to create a hierarchicalTaggedOpertions selector that wraps around a new filteredTaggedOperations selector. This would wrapp around the taggedOperations and would conditionally filter the taggedOperations. If we like we could refactor the old operations component to use the new filteredTaggedOperations selector. I think this would actually also improve the segregation of concerns in the original component too.
There was a problem hiding this comment.
Would this involve the spec selector reaching over into the layout selectors to get the current filter? Or would you pass a filter function down into the specified methods in the spec selector as an argument?
There was a problem hiding this comment.
Yeah just like I did:
But should use system.specSelectors.taggedOperatioms() to get the tagged operations.
There was a problem hiding this comment.
The most strict approach using the plugin system would be to create a wrapSelector within filter plugin to wrap spec's taggedOperations selector in case of having a filter enabled.
I actually would like to see this because this would be the right approach following the plugin system and segregation of concerns.
|
Closed in favor of #6 |
No description provided.